fix(#956): gate the pre-commit fallback on a 300s floor by fullsend-ai-coder[bot] · Pull Request #957 · fullsend-ai/agents

fullsend-ai-coder · 2026-08-23T01:16:40Z

Summary

Step 9b's STEP C direct-execution fallback (added by #946) is the expensive path: it pip installs each hook's tool at its pinned rev and runs the hooks one at a time. The 10% time gate that admits an agent into 9b was sized for the fast path — a plain pre-commit run over a few changed files. So an agent can clear the 10% gate, hit an infrastructure failure in STEP B, and enter the expensive fallback with too little budget left to finish it. Timing out mid-pip install produces no commit at all, which is worse than committing with the hooks disclosed as unrun.

This adds a second, stricter recheck at the point the fallback is entered, and reconciles the surrounding text that assumed only two ways to stop short of pre-commit.

The floor is 300s, flat

Issue #956 puts the risky window at roughly 150–250s remaining. harness/fix.yaml sets TIMEOUT_SECONDS: 1500 and harness/code.yaml sets 2100, so their 10% gates are 150s and 210s. A 300s floor sits above the window on both.

It is deliberately absolute rather than a fraction. What the floor guards costs about the same regardless of budget — it depends on how many remote hooks there are and what their additional_dependencies pull in, not on TIMEOUT_SECONDS. A 20% fraction would put the code agent's floor at 420s and skip the fallback across a 210s-wide band it can comfortably afford, reintroducing the over-skipping that #956 says condemned the old 40% gate — and it would keep growing if TIMEOUT_SECONDS were ever raised.

One consequence, stated in the code so it reads as a decision rather than an oversight: 300s can sit below 9c's uncapped 20% test-retry floor on the larger budget. That is intended — retrying tests is optional, running the fallback when there is time is the point.

Changes

skills/code-implementation/SKILL.md

9b STEP C: the RUN_FALLBACK check, with prose naming exactly which substeps to skip and the disclosure to write. Guards AGENT_START alongside TIMEOUT_SECONDS — unset, it evaluates as 0, which would put every run below the floor and turn the gate into a silent always-skip. Prints on every branch, so "proceed" is never signalled by an empty result.
9b lead: said "exactly two things let you stop short"; now three.
RULE 1 / 9c: the run budget is now stated as per iteration, not per sandbox. Since feat(#949): run pre-commit inside the code/fix validation loop #960 the validation loop classifies pre-commit-blocked as agent-fixable and dispatches a retry into the same sandbox, and R6 tells that retry to run pre-commit — so a session-wide lockout would have blocked the retry that exists to fix exactly this failure. 9c's own internal retries are still barred from re-entering 9b.
9c: dropped the "you already used your 2 pre-commit runs" rationale, which was false on the skip path.

skills/fix-review/SKILL.md

The same check, inlined. harness/fix.yaml mounts only skills/fix-review, so the fix agent cannot read the code-implementation text its 7b cross-reference points at — and fix-review is the agent fix-review can start step 7b too late to finish the STEP C direct-execution fallback #956 was filed about, with the smaller absolute margin.
The block states its precondition explicitly (only after an infrastructure failure — not after a pass, not after real hook errors) so it cannot produce a false "hooks were not run" disclosure on the success path.

Testing

Both snippets executed under set -euo pipefail across five scenarios each: 100s and 250s remaining → RUN_FALLBACK=0 + ::warning::; 400s and 2000s remaining → RUN_FALLBACK=1 + ::notice::; AGENT_START unset → no floor applied, RUN_FALLBACK=1 + ::notice::. Both files produce identical output on every case. Negative REMAINING correctly trips the skip.
make check-bundle passes (exit 0)
Whitespace, end-of-file newlines, and code-fence balance verified in both files
Rebased onto current main; the earlier conflict on the fix-review threshold line (< 40% → < 10%, from Fix-review skill pre-commit gate is too conservative, causing avoidable post-script failures #933) is resolved in favor of main's value

Known limitation

Below the floor the skip is all-or-nothing — it drops repo: local hooks along with the remote ones. That is deliberate and noted in both files: a local entry can fetch on its own (uvx, pip), so it is not reliably the cheap case, and 9c/7c's mandatory lint run still happens below the floor. Carving out on-PATH local hooks would be a reasonable follow-up.

Closes #956

fullsend-ai-review · 2026-08-23T01:18:37Z

🤖 Finished Review · ✅ Success · Started 1:18 AM UTC · Completed 1:36 AM UTC

Commit: 52d2080 · View workflow run →

fullsend-ai-review · 2026-08-23T01:36:04Z

Review

Findings

Medium

[protected-path] skills/code-implementation/SKILL.md, skills/fix-review/SKILL.md — Both changed files are under the skills/ protected path. The PR links to issue fix-review can start step 7b too late to finish the STEP C direct-execution fallback #956 and the changes are well-scoped to the issue's requested fix (adding a 300s fallback floor before the direct-execution fallback). Human approval is always required for protected-path changes, regardless of context.

Low

[edge-case] skills/code-implementation/SKILL.md — The 300s flat floor exactly equals the 20% retry floor for the fix-review agent (20% of 1500 = 300), and sits below the 20% retry floor for the code-implementation agent (20% of 2100 = 420). The interaction is documented and deliberate — the bash comment and surrounding prose explain why the floor is flat rather than proportional. No functional issue; the coincidence is noted for completeness.
[scope-expansion] skills/code-implementation/SKILL.md:805 — RULE 1's budget scope changed from "per session" to "per iteration." This is beyond issue fix-review can start step 7b too late to finish the STEP C direct-execution fallback #956's literal scope but is well-justified: PR feat(#949): run pre-commit inside the code/fix validation loop #960, now merged, classifies pre-commit-blocked as agent-fixable and dispatches a validation-loop retry. Under the old "per session" wording, that retry would be locked out of pre-commit.

Previous run

Review

Findings

Medium

[protected-path] skills/code-implementation/SKILL.md, skills/fix-review/SKILL.md — Both changed files are under the skills/ protected path. The PR links to issue fix-review can start step 7b too late to finish the STEP C direct-execution fallback #956 and the changes are well-scoped to the issue's requested fix (adding a 300s fallback floor before the direct-execution fallback). Human approval is always required for protected-path changes, regardless of context.

Low

[internal-consistency] skills/code-implementation/SKILL.md:673 — The inline comment says the 300s floor "can therefore sit below 9c's uncapped 20% retry floor." For the code-implementation agent (TIMEOUT_SECONDS=2100), 20% = 420s, so 300 < 420 and the claim holds. However, for fix-review (TIMEOUT_SECONDS=1500), 20% = 300s, making them equal, not "below." The fix-review copies the same comment at line 341. Practical impact is nil.
[structural-ordering-consistency] skills/fix-review/SKILL.md:363 — The RUN_FALLBACK branch ordering is reversed between the two skill files. In code-implementation (line 694), the skip case (RUN_FALLBACK=0) is presented first, followed by proceed (RUN_FALLBACK=1). In fix-review, RUN_FALLBACK=1 comes first, then RUN_FALLBACK=0.
[section-heading-consistency] skills/fix-review/SKILL.md:320 — The inline heading for the new section uses a different name from its counterpart in code-implementation. Code-implementation calls it "Time recheck before the fallback." while fix-review calls it "The fallback floor."
[scope-expansion] skills/code-implementation/SKILL.md:798 — RULE 1's budget scope changed from "per session" to "per iteration." This is beyond issue fix-review can start step 7b too late to finish the STEP C direct-execution fallback #956's literal scope but justified: the new floor creates a third skip path, and the per-session wording would prevent validation-loop retries from re-entering pre-commit to fix pre-commit-blocked failures.

Previous run (2)

Review

Findings

Medium

[protected-path] skills/code-implementation/SKILL.md, skills/fix-review/SKILL.md — Both changed files are under the skills/ protected path. The PR links to issue fix-review can start step 7b too late to finish the STEP C direct-execution fallback #956 and the changes are well-scoped to the issue's requested fix (adding a 300s fallback floor before the direct-execution fallback). Human approval is always required for protected-path changes, regardless of context.

Low

[terminology-consistency] skills/fix-review/SKILL.md — The AGENT_START guard explanation uses different wording between the two files: fix-review says "makes REMAINING a large negative number" while code-implementation says "ELAPSED becomes the raw epoch second, and every run would fall below the floor." Both are technically correct but explain the same mechanism differently, which could confuse readers cross-referencing the two skills.
[scope-expansion] skills/code-implementation/SKILL.md — The PR changes RULE 1's budget scope from "per session" to "per iteration." This is beyond issue fix-review can start step 7b too late to finish the STEP C direct-execution fallback #956's literal scope but is justified: the new floor creates a third skip path, and the per-session wording would prevent validation-loop retries from re-entering pre-commit to fix pre-commit-blocked failures.

Previous run (3)

Review

Findings

Medium

[protected-path] skills/code-implementation/SKILL.md, skills/fix-review/SKILL.md — Both changed files are under the skills/ protected path. The PR links to issue fix-review can start step 7b too late to finish the STEP C direct-execution fallback #956 and the changes are well-scoped to the issue's requested fix (adding a 20% time-floor recheck before the direct-execution fallback). Human approval is always required for protected-path changes, regardless of context.

Previous run (4)

Review

Findings

Medium

[protected-path] skills/code-implementation/SKILL.md, skills/fix-review/SKILL.md — Both changed files are under the skills/ protected path. The PR links to issue fix-review can start step 7b too late to finish the STEP C direct-execution fallback #956 and explains the rationale for the change (mid-fallback time recheck to prevent hard timeouts with zero artifacts). Human approval is always required for protected-path changes, regardless of context.

Low

[edge-case] skills/code-implementation/SKILL.md:567 — The bash snippet in the mid-fallback time recheck logs a notice when remaining time is below the 20% floor but has no explicit control-flow mechanism (e.g., else clause, early exit, or flag variable) to prevent the subsequent fallback steps from executing. The comment # Proceed to 9c — disclose in commit message provides intent, and the surrounding prose instructs the agent to skip the fallback, so this is likely sufficient for the LLM agent consumer. Adding an explicit else clause wrapping the remaining fallback steps would make the intent unambiguous.

fullsend-ai-review · 2026-08-23T13:44:57Z

🤖 Finished Review · ✅ Success · Started 1:44 PM UTC · Completed 1:58 PM UTC

Commit: c094b5e · View workflow run →

fullsend-ai-review · 2026-08-23T14:02:50Z

🤖 Review · ⚠️ Cancelled · Started 2:02 PM UTC · Ended 2:13 PM UTC

Commit: 9268f08 · View workflow run →

fullsend-ai-review · 2026-08-23T14:15:20Z

🤖 Review · ⚠️ Cancelled · Started 2:15 PM UTC · Ended 2:31 PM UTC

Commit: d1d9dbb · View workflow run →

fullsend-ai-review · 2026-08-23T14:32:51Z

🤖 Finished Review · ✅ Success · Started 2:32 PM UTC · Completed 2:49 PM UTC

Commit: 06929d3 · View workflow run →

fullsend-ai-review · 2026-08-23T14:52:53Z

🤖 Finished Review · ✅ Success · Started 2:52 PM UTC · Completed 3:10 PM UTC

Commit: 7f288fb · View workflow run →

The direct-execution fallback in step 9b STEP C (introduced by #946) installs each hook's tool via pip and runs the hooks one at a time. The 10% gate that admits an agent into 9b was sized for the fast path -- a plain `pre-commit run` on a few files -- so an agent can clear it, hit an infrastructure failure, and enter the expensive path with too little budget left to finish it. Timing out mid-install produces no commit at all, which is worse than committing with the hooks disclosed as unrun. Re-check the budget at the point the fallback is entered, against a flat 300s floor. The floor is absolute rather than a fraction because what it guards costs about the same whatever the budget is; scaling it would only over-skip on the larger budget, and 300s clears the 150-250s window #956 identifies on both harnesses. Below the floor, set RUN_FALLBACK=0, skip the fallback substeps, and go to 9c with the skip disclosed. The fast path keeps its 10% gate. The check guards AGENT_START as well as TIMEOUT_SECONDS: unset, it evaluates as 0, which would put every run below the floor and turn the gate into a silent always-skip. It prints on every branch so the proceed path is not signalled by an empty result. Reconcile the surrounding text, which previously described a world with only two ways to stop short of pre-commit: 9b's lead, the threshold rationale, RULE 1's run accounting, and 9c's prohibition on re-entering 9b, whose stated reason ("you already used your 2 runs") was false on the skip path. State the run budget as per-iteration rather than per-sandbox: since #960 the validation loop treats pre-commit-blocked as agent-fixable and dispatches a retry that R6 tells to run pre-commit, so a session-wide lockout would block the retry that exists to fix it. Carry the same check into the fix-review skill inline rather than by cross-reference. harness/fix.yaml mounts only skills/fix-review, so the fix agent cannot read the code-implementation text its step 7b points at -- and fix-review is the agent this issue was filed about. Closes #956 Assisted-by: Claude (fix, review), Grok (review)

fullsend-ai-review · 2026-08-23T15:14:10Z

🤖 Finished Review · ✅ Success · Started 3:14 PM UTC · Completed 3:30 PM UTC

Commit: 363a7fc · View workflow run →

fullsend-ai-review

See the review comment for full details.

waynesun09

Approving as code owner (@fullsend-ai/core).

Reviewed across three multi-model rounds plus four bot rounds; head 363a7fc is 14/14 green. All remaining bot findings are LOW and either fixed in 363a7fc or documented in-file as deliberate (the flat 300s floor vs the 20% retry floors; the per-iteration budget rescoping required so #960's pre-commit-blocked retry is not locked out of step 9b/7b).

fullsend-ai-coder Bot requested a review from a team as a code owner August 23, 2026 01:16

fullsend-ai-coder Bot added the ready-for-review label Aug 23, 2026

fullsend-ai-coder Bot assigned waynesun09 Aug 23, 2026

This comment was marked as outdated.

Sign in to view

fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Aug 23, 2026

waynesun09 force-pushed the agent/956-step-c-time-recheck branch from 52d2080 to c094b5e Compare August 23, 2026 13:43

waynesun09 changed the title ~~fix(#956): add mid-fallback time recheck before STEP C direct execution~~ fix(#956): gate the pre-commit fallback on a 20% time floor Aug 23, 2026

waynesun09 force-pushed the agent/956-step-c-time-recheck branch from c094b5e to 9268f08 Compare August 23, 2026 14:00

waynesun09 changed the title ~~fix(#956): gate the pre-commit fallback on a 20% time floor~~ fix(#956): gate the pre-commit fallback on a time floor Aug 23, 2026

waynesun09 force-pushed the agent/956-step-c-time-recheck branch from 9268f08 to d1d9dbb Compare August 23, 2026 14:13

waynesun09 changed the title ~~fix(#956): gate the pre-commit fallback on a time floor~~ fix(#956): gate the pre-commit fallback on a 300s floor Aug 23, 2026

waynesun09 force-pushed the agent/956-step-c-time-recheck branch from d1d9dbb to 06929d3 Compare August 23, 2026 14:30

This comment was marked as outdated.

Sign in to view

waynesun09 force-pushed the agent/956-step-c-time-recheck branch from 06929d3 to 7f288fb Compare August 23, 2026 14:50

This comment was marked as outdated.

Sign in to view

waynesun09 force-pushed the agent/956-step-c-time-recheck branch from 7f288fb to 363a7fc Compare August 23, 2026 15:12

fullsend-ai-review Bot reviewed Aug 23, 2026

View reviewed changes

Comment thread skills/code-implementation/SKILL.md

waynesun09 approved these changes Aug 26, 2026

View reviewed changes

waynesun09 added this pull request to the merge queue Aug 26, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 26, 2026

Uh oh!

Conversation

fullsend-ai-coder Bot commented Aug 23, 2026 • edited by waynesun09 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

The floor is 300s, flat

Changes

Testing

Known limitation

Uh oh!

fullsend-ai-review Bot commented Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Review

Findings

Medium

Low

Uh oh!

This comment was marked as outdated.

Uh oh!

fullsend-ai-review Bot commented Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

fullsend-ai-review Bot commented Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

fullsend-ai-review Bot commented Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

waynesun09 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

fullsend-ai-coder Bot commented Aug 23, 2026 •

edited by waynesun09

Loading

fullsend-ai-review Bot commented Aug 23, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 23, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 23, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 23, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 23, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 23, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 23, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 23, 2026 •

edited

Loading